Documentation preview for this PR (built with commit 5784784; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

Actually, looking at e.g. /IPython/core/magics/execution.py, the convention is to use IPython.core.magic.Magics.parse_options, which has a getopt.getopt()-like interface.

Is it worth it to change it to be consistent?

On the other hand getopt documentation says

Deprecated since version 3.13: The getopt module is soft deprecated and will not be developed further; development will continue with the argparse module.

and IPython parse_options uses getopt internally.

(are you sure you don't mean to approve the other PR and leave the comment on documentation on this one instead?)

Actually, looking at e.g. /IPython/core/magics/execution.py, the convention is to use IPython.core.magic.Magics.parse_options, which has a getopt.getopt()-like interface.

Is it worth it to change it to be consistent?

On the other hand getopt documentation says

Deprecated since version 3.13: The getopt module is soft deprecated and will not be developed further; development will continue with the argparse module.

and IPython parse_options uses getopt internally.

I think we should be future-proof and argparseis the future.

(are you sure you don't mean to approve the other PR and leave the comment on documentation on this one instead?)

Either way is okay with me. Just let me know which one depends on which.

For our simple arguments, are there differences in argument style between getopt and argparse?

On the user side, it would looks mostly the same. But the code would look like this (i.e. more manual handling)

        boolean_options = {
                "m": "compile-message",
                "c": "use-cache",
                "l": "create-local-c-file",
                "a": "annotate",
                "s": "sage-namespace",
                "o": "create-local-so-file",
                }
        args, rest = self.parse_options(line, "v:" + "".join(boolean_options), "verbose=",
                                              *boolean_options.values(),
                                              *["no-" + k for k in boolean_options.values()])
        from IPython.core.error import UsageError
        if rest:
            raise UsageError(f"unrecognized arguments: {rest}")
        args_dict = dict(args)
        if "v" in args_dict:
            args_dict["verbose"] = args_dict["v"]
            del args_dict["v"]
        if "verbose" in args_dict:
            args_dict["verbose"] = int(args_dict["verbose"])
        for k in [*args_dict]:
            if k in boolean_options:
                args_dict[boolean_options[k]] = True
                del args_dict[k]
            elif k in boolean_options.values():
                args_dict[k] = True
            elif k.startswith("no-"):
                args_dict[k[3:]] = False
                del args_dict[k]
        args_dict = {k.replace("-", "_"): v for k, v in args_dict.items()}
        return cython_compile(cell, **args_dict)

or

        args, rest = self.parse_options(
                line, "v:mclaso", "verbose=",
                "compile-message", "use-cache", "create-local-c-file", "annotate", "sage-namespace", "create-local-so-file",
                "no-compile-message", "no-use-cache", "no-create-local-c-file", "no-annotate", "no-sage-namespace", "no-create-local-so-file",
                )
        from IPython.core.error import UsageError
        if rest:
            raise UsageError(f"unrecognized arguments: {rest}")
        args_dict = {}
        for k, v in args.items():
            if k in ("v", "verbose"):
                args_dict["verbose"] = int(v)
            elif k in ("m", "compile-message"):
                args_dict["compile_message"] = True
            elif k in ("c", "use-cache"):
                args_dict["use_cache"] = True
            elif k in ("l", "create-local-c-file"):
                args_dict["create_local_c_file"] = True
            elif k in ("a", "annotate"):
                args_dict["annotate"] = True
            elif k in ("s", "sage-namespace"):
                args_dict["sage_namespace"] = True
            elif k in ("o", "create-local-so-file"):
                args_dict["create_local_so_file"] = True
            elif k.startswith("no-"):
                args_dict[k[3:].replace("-", "_")] = False
            else:
                raise UsageError(f"unrecognized argument: {k}")
        return cython_compile(cell, **args_dict)

Only the error message would look different

sage: %%cython -v1 --annotate --no-sage-namespace --xx
....: def f():
....:     print('test')
....: 
UsageError: option --xx not recognized ( allowed: "v:mclaso" verbose= compile-message use-cache crea
te-local-c-file annotate sage-namespace create-local-so-file no-compile-message no-use-cache no-crea
te-local-c-file no-annotate no-sage-namespace no-create-local-so-file)

which aligns with existing IPython magics

sage: %run -abc
UsageError: option -a not recognized ( allowed: "nidtN:b:pD:l:rs:T:em:G" )

I see. Aligning with existing ipython magics is good for consistency. Consistency is more visible to users. Hence I would value consistency more than future-proof. Anyway, I will leave the decision to you.

I think there's also the part that using parse_arguments() instead of argparse makes the code significantly more complicated. (there's the BooleanOptionalAction thing which makes it even shorter but is Python 3.9 only.)

I don't think consistency is a big issue, now that I raise UsageError instead of argparse.ArgumentError.

Done? LGTM.

Done? LGTM.

Can you change the label to positive-review then? Thanks.

(not to try to bikeshedding the issue, but: do you think it's a good idea to specify all short options like that? It may take up space for other more sensical options later. I'd say -v and -a is obvious, but others maybe not so much… Later maybe we could use -w for view-annotate (what's the logic) and either -p for preparse or -s for sage-preparse (because of the pyx versus spyx extension).)

not to try to bikeshedding the issue,

Setting up a bike shed in a lot beside of a nuclear powerplant is perhaps a trivial thing. But in software engineering, I think small things are not trivial; they affect many people for a long time.

do you think it's a good idea to specify all short options like that?

Alternatively, you may just give a couple of examples with useful options.

Will you merge this branch to #38946?

When you are done with this PR, you may set "positive review".

will merge later when it's stable.

No, what I mean is should short option exists for every long option --- for example -s for --sage-namespace might not make sense, and later we may want -s to stand for e.g. --sage-preparse instead which is admittedly more common.

Problem is once a short option is taken for something it cannot be taken for something else (which in retrospect might make more sense later) without like a year of deprecation so…

I don't think I have permission to set review labels myself.

will merge later when it's stable.

OK.

No, what I mean is should short option exists for every long option --- for example -s for --sage-namespace might not make sense, and later we may want -s to stand for e.g. --sage-preparse instead which is admittedly more common.

Problem is once a short option is taken for something it cannot be taken for something else (which in retrospect might make more sense later) without like a year of deprecation so…

I see. I agree that short option should exist only for frequent long options.

I don't think I have permission to set review labels myself.

OK. Let me know when you are done.

I realize I don't know which long option are common… but these 2 are definitely not common (they defaults to True).

Chances are it doesn't matter that much anyway. I'm done with this.

Actually…

you can %load_ext cython which gives another %%cython which looks much more advanced.

http://docs.cython.org/en/latest/src/userguide/source_files_and_compilation.html#compiling-with-a-jupyter-notebook

Should we just deprecate sage's %%cython then? (though the cython extension version has slightly different behavior from the sage version, with slightly different default e.g. sage has -w by default)

at the very least I should delete the command-line arguments that conflicts with the cython version.

you can %load_ext cython which gives another %%cython which looks much more advanced.

http://docs.cython.org/en/latest/src/userguide/source_files_and_compilation.html#compiling-with-a-jupyter-notebook

Hmm I didn't know that (or completely forgot :-) Perhaps that is why sage's cython magic was not developed much by now.

Should we just deprecate sage's %%cython then? (though the cython extension version has slightly different behavior from the sage version, with slightly different default e.g. sage has -w by default)

I don't know. It all depends on how sage's cython magic compares with ipython's. Anyway a major merit of sage's cython magic is that it is available by default. Hence I think just removing it is a step backward.

at the very least I should delete the command-line arguments that conflicts with the cython version.

Yes. We should align sage's cython magic with ipython's at least in user interface. As for this pr, for example, we should not define an argument with a different name but with the same function. This would confuse users.

In the long run, sage's cython magic may be redefined as a wrapper of ipython's cython magic, loaded by default and with some additional conveniency arguments for sage users.

Anyway a major merit of sage's cython magic is that it is available by default.

That is easily bypassable by loading the extension at startup by Sage.

Anyway it looks like the semantic of Sage's cell magic is a bit different from Cython's one (e.g. Sage's one suppress warnings by default) so yes we can just keep both, and maybe mention the existence of IPython's one somewhere in the documentation.